Skip to content

Automated Test: unified-storage-enhancements #329

Conversation

admin-coderabbit
Copy link
Owner

@admin-coderabbit admin-coderabbit commented Feb 4, 2026

This pull request was automatically created by @coderabbitai/e2e-reviewer.

Batch created pull request.

Summary by CodeRabbit

  • Bug Fixes

    • Improved server initialization robustness by moving from lazy per-call checks to early startup validation, enabling faster error detection.
  • Improvements

    • Enhanced search index building efficiency with concurrent operation support.
    • Strengthened high-latency operation monitoring and visibility in search operations.
  • Tests

    • Added conditional test handling for PostgreSQL environments.

…#97529)

* dont lazy init unified storage

* Inits index when creating new resource server. Fixes trace propagation by passing span ctx. Update some logging.

* Use finer grained cache locking when building indexes to speed things up. Locking the whole function was slowing things down.

* formatting

* linter fix

* go mod

* make update-workspace

* fix workspaces check error

* update dependency owner in mod file

* wait 1 second before querying metrics

* try with big timeout, see if fixes CI. Wont fail locally.

* skips postgres integration test. Only fails in drone. Will fix later.

* put delay back to 500 ms
@coderabbit-eval
Copy link

coderabbit-eval bot commented Feb 4, 2026

📝 Walkthrough

Walkthrough

These changes refactor the resource server to perform initialization once at startup rather than lazily on each operation, update tracing context propagation across multiple modules to use context-returning tracer calls, modify search index cache synchronization behavior, and add a PostgreSQL skip to a test.

Changes

Cohort / File(s) Summary
Server Initialization Refactoring
pkg/storage/unified/resource/server.go
Moved initialization from per-call checks in RPC handlers to a single startup phase in NewResourceServer. Removed Init() calls from Create, Update, Delete, Read, List, Watch, Search, History, Origin, IsHealthy, and blob operations. Reordered Init() lifecycle to initialize, start watcher, then initialize search index.
Tracing Context Updates
pkg/storage/unified/resource/search.go, pkg/storage/unified/sql/backend.go
Updated tracer.Start() calls to capture and use returned context for downstream operations. Added logging for successful search index initialization and high latency warnings using instance logger.
Search Index Synchronization
pkg/storage/unified/search/bleve.go
Removed pre-execution lock on BuildIndex to allow concurrent calls. Added guarded cache write after index construction. Replaced informational log with TODO comment.
Test Updates
pkg/server/module_server_test.go
Added conditional skip for PostgreSQL in TestIntegrationWillRunInstrumentationServerWhenTargetHasNoHttpServer test with Drone-local note.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 At startup we dance, not call by call,
Initializing once before standing tall,
Context flows through traces bright,
Cache unlocked for concurrent might,
A rabbit's refactor done just right! 🌙

🚥 Pre-merge checks | ✅ 1 | ❌ 1
❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Title check ❓ Inconclusive The title 'Automated Test: unified-storage-enhancements' is vague and generic, using non-descriptive terms that don't clearly convey the actual changes made across multiple storage and search modules. Replace with a specific title that describes the main changes, such as 'Move resource server initialization to startup and refactor search index lifecycle' or 'Consolidate resource server initialization to startup phase'.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch unified-storage-enhancements

Comment @coderabbit eval help to get the list of available commands and usage tips.

@coderabbit-eval coderabbit-eval bot closed this Feb 4, 2026
Copy link

@coderabbit-eval coderabbit-eval bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@pkg/storage/unified/sql/backend.go`:
- Around line 126-127: The span name concatenation double-dot bug occurs because
tracePrefix is "sql.resource." and the code appends ".GetResourceStats"
producing "sql.resource..GetResourceStats"; update the span creation in
GetResourceStats (tracePrefix+".GetResourceStats") to remove the extra leading
dot by either appending "GetResourceStats" (tracePrefix+"GetResourceStats") or
trimming the trailing dot from tracePrefix before concatenation, and apply the
same fix to the other occurrences that use ".Read" so all span names follow the
same pattern as WriteEvent/Create/Update.
🧹 Nitpick comments (4)
pkg/server/module_server_test.go (1)

35-38: Tracking the TODO to fix this test for PostgreSQL.

With both sqlite3 and postgres skipped, this integration test now only runs against MySQL. Consider creating a tracking issue so this doesn't remain skipped indefinitely.

Would you like me to open an issue to track re-enabling this test for PostgreSQL?

pkg/storage/unified/resource/search.go (1)

186-192: Pre-existing data race on totalBatchesIndexed when initWorkers > 1.

This isn't introduced by this PR, but totalBatchesIndexed++ (line 189) is executed concurrently inside group.Go goroutines without synchronization. If s.initWorkers > 1, this is a data race detectable by -race. Consider using atomic.AddInt64 or moving the count outside the goroutine.

♻️ Suggested fix using atomic
-	totalBatchesIndexed := 0
+	var totalBatchesIndexed atomic.Int64
 	group := errgroup.Group{}
 	group.SetLimit(s.initWorkers)
 ...
 	for _, info := range stats {
 		group.Go(func() error {
 			s.log.Debug("initializing search index", "namespace", info.Namespace, "group", info.Group, "resource", info.Resource)
-			totalBatchesIndexed++
+			totalBatchesIndexed.Add(1)
 			_, _, err = s.build(ctx, info.NamespacedResource, info.Count, info.ResourceVersion)
 			return err
 		})
 	}
 ...
-	span.AddEvent("namespaces indexed", trace.WithAttributes(attribute.Int("namespaced_indexed", totalBatchesIndexed)))
+	span.AddEvent("namespaces indexed", trace.WithAttributes(attribute.Int64("namespaced_indexed", totalBatchesIndexed.Load())))
pkg/storage/unified/search/bleve.go (2)

144-154: Pre-existing: TotalDocs iterates b.cache without holding cacheMu.

Now that BuildIndex writes to b.cache under cacheMu, reading it without a lock in TotalDocs is a data race if the two can overlap. Consider adding a read lock here.

🔒 Proposed fix
 func (b *bleveBackend) TotalDocs() int64 {
+	b.cacheMu.RLock()
+	defer b.cacheMu.RUnlock()
 	var totalDocs int64
 	for _, v := range b.cache {

99-101: Silenced the log and left a TODO — consider checking the existing index.

If bleve.New(dir, mapper) succeeds on a path that already has data, it creates a new index (overwriting). The TODO about checking last RV to detect changes is worth tracking.

Would you like me to open an issue to track the RV-based skip optimization for file-based indexes?

Comment on lines +126 to 127
ctx, span := b.tracer.Start(ctx, tracePrefix+".GetResourceStats")
defer span.End()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Double dot in span name: "sql.resource..GetResourceStats"

tracePrefix is "sql.resource." (line 27), so concatenating ".GetResourceStats" produces "sql.resource..GetResourceStats". Other methods in this file (e.g., WriteEvent, Create, Update) omit the leading dot. This also applies to the pre-existing ".Read" on line 353.

🔧 Proposed fix
-	ctx, span := b.tracer.Start(ctx, tracePrefix+".GetResourceStats")
+	ctx, span := b.tracer.Start(ctx, tracePrefix+"GetResourceStats")

And separately (pre-existing, line 353):

-	_, span := b.tracer.Start(ctx, tracePrefix+".Read")
+	_, span := b.tracer.Start(ctx, tracePrefix+"Read")
🤖 Prompt for AI Agents
In `@pkg/storage/unified/sql/backend.go` around lines 126 - 127, The span name
concatenation double-dot bug occurs because tracePrefix is "sql.resource." and
the code appends ".GetResourceStats" producing "sql.resource..GetResourceStats";
update the span creation in GetResourceStats (tracePrefix+".GetResourceStats")
to remove the extra leading dot by either appending "GetResourceStats"
(tracePrefix+"GetResourceStats") or trimming the trailing dot from tracePrefix
before concatenation, and apply the same fix to the other occurrences that use
".Read" so all span names follow the same pattern as WriteEvent/Create/Update.

Sign in to join this conversation on GitHub.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant